Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graphql: Minor delete mutation msg fix #5316

Merged
merged 6 commits into from
May 12, 2020

Conversation

vardhanapoorv
Copy link
Contributor

@vardhanapoorv vardhanapoorv commented Apr 28, 2020

This PR changes the delete msg reported for the case when no nodes are deleted to No nodes were deleted. In addition, it corrects some tests which were wrong earlier.

Fixes #GRAPHQL-423.

This change is Reviewable

Docs Preview: Dgraph Preview

@vardhanapoorv vardhanapoorv requested review from manishrjain and a team as code owners April 28, 2020 08:32
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

But looks like there's some other places in tests where that message was being relied on.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

@abhimanyusinghgaur abhimanyusinghgaur changed the title Minor delete mutation msg fix graphql: Minor delete mutation msg fix May 12, 2020
@abhimanyusinghgaur abhimanyusinghgaur added the area/graphql Issues related to GraphQL support on Dgraph. label May 12, 2020
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!! Love it.

Merge this and then either fix or remove that TODO.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @vardhanapoorv)


ee/acl/acl_test.go, line 90 at r2 (raw file):

func deleteUser(t *testing.T, accessToken, username string, confirmDeletion bool) {
	// TODO - Verify that only one uid got deleted once numUids are returned as part of the payload.

Can we also fix this TODO ? Just merge this PR and do separately.


ee/acl/acl_test.go, line 169 at r2 (raw file):

func TestGetCurrentUser(t *testing.T) {
	accessJwt, _ := testutil.GrootHttpLogin(adminEndpoint)

Thanks - always good to see code get cleaned up.


ee/acl/acl_test.go, line 199 at r2 (raw file):

	// adding the user again should fail
	accessJwt, _ := testutil.GrootHttpLogin(adminEndpoint)

again nice simplification. - looks like there has been some repeated code in here.


graphql/e2e/common/mutation.go, line 2298 at r2 (raw file):

func cleanupStarwars(t *testing.T, starshipID, humanID, droidID string) {
	// Delete everything
	multiMutationParams := &GraphQLParams{

love it!

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @vardhanapoorv)


ee/acl/acl_test.go, line 90 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Can we also fix this TODO ? Just merge this PR and do separately.

Ok, will do it separately.

@abhimanyusinghgaur abhimanyusinghgaur merged commit 47f2f51 into master May 12, 2020
@abhimanyusinghgaur abhimanyusinghgaur deleted the apoorv/delete-mutation-msg-fix branch May 12, 2020 11:47
abhimanyusinghgaur added a commit that referenced this pull request Jun 3, 2020
This PR changes the delete msg reported for the case when no nodes are deleted to No nodes were deleted. In addition, it corrects some tests which were wrong earlier.

Fixes #GRAPHQL-423.

Co-authored-by: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com>
(cherry picked from commit 47f2f51)

# Conflicts:
#	ee/acl/acl_test.go
#	graphql/e2e/auth/auth_test.go
#	graphql/e2e/common/mutation.go
#	graphql/resolve/mutation.go
abhimanyusinghgaur added a commit that referenced this pull request Jun 3, 2020
This PR changes the delete msg reported for the case when no nodes are deleted to No nodes were deleted. In addition, it corrects some tests which were wrong earlier.

Fixes #GRAPHQL-465.

(cherry picked from commit 47f2f51)

# Conflicts:
#	ee/acl/acl_test.go
#	graphql/e2e/auth/auth_test.go
#	graphql/e2e/common/mutation.go
#	graphql/resolve/mutation.go
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
This PR changes the delete msg reported for the case when no nodes are deleted to No nodes were deleted. In addition, it corrects some tests which were wrong earlier.

Fixes #GRAPHQL-423.

Co-authored-by: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants